Hey @jleni can you also link to a PR to ZondaX/ledger-cosmos-go with the changes that you are importing into the SDK? We can't really reason about the changes if we don't have that. Also this PR needs a rebase.

Yes, the conflict appeared once another PR was merged last night. No worries, the PR is still WIP.

@jackzampolin I have now included a longer technical explanation for all the changes that were made with links to each respective layer and PR.

Codecov Report

Merging #3431 into develop will increase coverage by 6.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3431      +/-   ##
===========================================
+ Coverage    52.97%   59.21%   +6.23%     
===========================================
  Files          142      134       -8     
  Lines        10363     9940     -423     
===========================================
+ Hits          5490     5886     +396     
+ Misses        4550     3680     -870     
- Partials       323      374      +51

As discussed with @cwgoes @jackzampolin

I have noticed something that may result in problems. At the moment, when a key is requested to a ledger device, a PrivKeyLedgerSecp256k1 object keeps the device together with the keys. However, this object is actually a USB resource that is never released.

At the moment, there are a few problems in gaia and still in this PR:

• discovery opens the resource but never closes it... calling discovery twice fails as the resource has not been released.
• the cached usb resource can fail in many ways (disconnects, etc.)
• we might need to add code to reconnect, verify that it is the same device, etc.
• PrivKeyLedgerSecp256k1 gets serialized to amino I am sure it is skipping the connection object, but still.. it feels odd.

We agreed on refactoring gaia's code, so resources are released ASAP and pks are rechecked before signing. As PKs are checked, it should be possible to detect if devices are swapped or there are multiple devices and the incorrect one is re-discovered.

In addition to these changes, I have noticed that we have a mix of DER vs BER signatures. I will try to unify this a bit too to avoid confusion.

This PR just updated dependancies.

True, for now the changes are in other layers as described above.
I will soon push all the other local changes I made that apply to the SDK.
Once the fixes in the dependencies got integrated, there are other reasons why there might be connection issues. Also unit tests for ledger in the SDK are quite weak. Actually, they are skipped at the moment.

@jleni Looking forward to unit testing for this really crucial usecase. Thank you for adding them!

I would say this is not the PR/issue for that.. but once #3461 and #3431 are merged, I would open another issue to add cli tests that check a real ledger device too.

@jackzampolin Given that these fixes have uncovered an existing incompatibility between DER and BER signatures, I would suggest considering this PR for prelaunch: without these changes, signatures will be invalid.

It seems an unrelated issue hit integration_tests here:

    gobash.go:80: Running gaiad start --home=/tmp/gaia_integration_TestGaiaCLISubmitProposal_210313979/.gaiad --rpc.laddr=tcp://0.0.0.0:32893 --p2p.laddr=tcp://0.0.0.0:38125
panic: Get http://localhost:32893/block: read tcp 127.0.0.1:53250->127.0.0.1:32893: read: connection reset by peer [recovered]
	panic: Get http://localhost:32893/block: read tcp 127.0.0.1:53250->127.0.0.1:32893: read: connection reset by peer

Should I open another issue for that? I can try rerunning but there is a panic to analyse there anyway.

Here is the link to the CI error:
https://circleci.com/gh/ZondaX/cosmos-sdk/343

Running integration_tests again and they passed. There is definitely a sporadic unrelated issue in integration_tests that needs to be looked at.

@jackzampolin @alessio Fixed a conflict in Pending.md.
Hopefully, this is also ready to go.

@jackzampolin @alessio This PR and #3461 have changes in similar areas. Whichever you merge first will result in changes on the other side.
I would say, we merge #3461 first before it goes stale and I adjust this PR accordingly.

@jleni Per your suggestion I've merged #3461 first. This now has merge conflicts.

Yes, thanks! I will adjust this in the next hour